Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hook up metadata filter on the_preview #40

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mboynes
Copy link

@mboynes mboynes commented Mar 16, 2018

Resolves #30. Needs testing.

@adamsilverstein
Copy link
Owner

Hi @mboynes - thanks for the PR and apologies for the long delay reviewing. I am finally getting to responding now. Do you have a description of steps to reproduce/how you tested this?

@adamsilverstein
Copy link
Owner

related: #37

@mboynes
Copy link
Author

mboynes commented Mar 15, 2019

Hey @adamsilverstein, no worries on the delay! Thanks for looking it over.

At the time, I tested it by making meta changes to an article, previewing that article, and confirming that my changes had been made. I haven't tried it since, so especially with the introduction of Gutenberg, I can't say for certain that this still works. Though I'd be surprised if it didn't.

I hadn't noticed #37 at the time, this accomplishes the same task. I would offer that, assuming it still works, this PR may be moderately more performant, since it's not checking is_preview() on every meta call.

Let me know if I can be of any further help!

@adamsilverstein
Copy link
Owner

this PR may be moderately more performant, since it's not checking is_preview() on every meta call.

Good point, this is a better approach. Going to do some testing in 4.9/classic and 5.2/GB to verify functionality.

Thanks again for the PR @mboynes!

@toddmilliken
Copy link

Awesome PR @mboynes!

I borrowed some of this preview code for a project and it works great with one little change for me. This might have just been my environment, but the following condition was not evaluating to true for me:

'revision' === $post->post_type

Instead, the post_type value was 'page'. This might have been because the get_post() call on line 199 might be using the post ID being viewed, rather than using the ID from the get_post_meta() call.

So I think we might want to change:

to:

$post = get_post( $object_id );

so it queries the revision post rather than the parent post?

That seemed to solve the issue for me, and this PR worked wonders. Let me know if you want me to make a PR into your existing PR! 😄

@adamsilverstein
Copy link
Owner

@toddmilliken that improvement sounds helpful, I hope to get back to working on this again soon :)

Copy link

@grappler grappler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested this and it works as expected once the conflict is fixed.

@adamsilverstein
Copy link
Owner

@grappler thanks for confirming, still on my radar to test this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preview Metadata Changes
4 participants